[clang-tidy] Improved readability redundant casting with enums #111424

felix642 · 2024-10-07T19:35:03Z

Fixed false negatives with readability-redundant-casting when the underlying types are the same and the option IgnoreTypeAliases is set to true.

Fixes #111137

llvmbot · 2024-10-07T19:35:41Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Félix-Antoine Constantin (felix642)

Changes

Fixed false negatives with readability-redundant-casting when the underlying types are the same and the option IgnoreTypeAliases is set to true.

Fixes #111137

Full diff: https://github.com/llvm/llvm-project/pull/111424.diff

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/RedundantCastingCheck.cpp (+8-1)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
(modified) clang-tools-extra/test/clang-tidy/checkers/readability/redundant-casting.cpp (+20)

diff --git a/clang-tools-extra/clang-tidy/readability/RedundantCastingCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantCastingCheck.cpp
index b9ff0e81cbc522..ce0b32439c43a9 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantCastingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantCastingCheck.cpp
@@ -40,8 +40,15 @@ static bool areTypesEqual(QualType S, QualType D) {
 
 static bool areTypesEqual(QualType TypeS, QualType TypeD,
                           bool IgnoreTypeAliases) {
-  const QualType CTypeS = TypeS.getCanonicalType();
   const QualType CTypeD = TypeD.getCanonicalType();
+
+  QualType CTypeS;
+  const auto *const EnumTypeS = TypeS->getAs<EnumType>();
+  if(EnumTypeS != nullptr && !EnumTypeS->getDecl()->isScoped())
+    CTypeS = EnumTypeS->getDecl()->getIntegerType();
+  else
+    CTypeS = TypeS.getCanonicalType();
+
   if (CTypeS != CTypeD)
     return false;
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3e051c7db6adcc..32bae30821092f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -222,6 +222,12 @@ Changes in existing checks
   by adding the option `UseUpperCaseLiteralSuffix` to select the
   case of the literal suffix in fixes.
 
+- Improved :doc:`readability-redundant-casting
+  <clang-tidy/checks/readability/redundant-casting>` check by fixing
+  false negatives related to ``enum`` when setting ``IgnoreTypeAliases``
+  to true.
+
+
 - Improved :doc:`readability-redundant-smartptr-get
   <clang-tidy/checks/readability/redundant-smartptr-get>` check to
   remove `->`, when redundant `get()` is removed.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-casting.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-casting.cpp
index 30cac6bd5cca06..ed49f32364cba8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-casting.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-casting.cpp
@@ -221,3 +221,23 @@ void testRedundantDependentNTTPCasting() {
   // CHECK-MESSAGES: :[[@LINE-4]]:25: note: source type originates from referencing this non-type template parameter
   // CHECK-FIXES: {{^}}  T a = V;
 }
+
+enum E1 : char {};
+enum class E2 : char {};
+enum E3 {};
+
+void testEnum(E1 e1, E2 e2, E3 e3){
+  char a = static_cast<char>(e1);
+  // CHECK-MESSAGES-ALIASES: :[[@LINE-1]]:12: warning: redundant explicit casting to the same type 'char' as the sub-expression, remove this casting [readability-redundant-casting]
+  // CHECK-MESSAGES-ALIASES: :[[@LINE-3]]:18: note: source type originates from referencing this parameter
+  // CHECK-FIXES-ALIASES: {{^}}  char a = e1;
+
+  unsigned int d = static_cast<unsigned int>(e3);
+  // CHECK-MESSAGES-ALIASES: :[[@LINE-1]]:20: warning: redundant explicit casting to the same type 'unsigned int' as the sub-expression, remove this casting [readability-redundant-casting]
+  // CHECK-MESSAGES-ALIASES: :[[@LINE-8]]:32: note: source type originates from referencing this parameter
+  // CHECK-FIXES-ALIASES: {{^}}  unsigned int d = e3;
+
+  char b = static_cast<char>(e2);
+  char c = static_cast<char>(e3);
+  E1 e = static_cast<E1>('0');
+}

github-actions · 2024-10-07T19:41:10Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Fixed false negatives with readability-redundant-casting when the underlying types are the same and the option IgnoreTypeAliases is set to true. Fixes llvm#111137

clang-tools-extra/docs/ReleaseNotes.rst

5chmidti · 2024-10-08T20:45:21Z

Fixed false negatives with readability-redundant-casting when the underlying types are the same and the option IgnoreTypeAliases is set to true.

To me,

Fixed false negatives with readability-redundant-casting when casting an enum to its underlying type and the option IgnoreTypeAliases is set to true.

sounds better as the commit description.

The CI failure is probably because the implicit type for E3 is not unsigned int on Windows.

Code Review & Removed non-portable test

[clang-tidy] Improved readability redundant casting with enums Fixed false negatives with readability-redundant-casting when casting an enum to its underlying type and the option IgnoreTypeAliases is set to true. Fixes llvm#111137

felix642 · 2024-11-02T16:14:07Z

sounds better as the commit description.

I agree, thank you for the suggestion.

The CI failure is probably because the implicit type for E3 is not unsigned int on Windows.

You are right. I could not find a way to make this test platform independent. I would either have to include <type_traits> or add preprocessor definitions to check for the OS (which might also break in the future). So I have decided to remove this test, since it didn't really add any value to the test suite.

clang-tools-extra/docs/ReleaseNotes.rst

Removed excessive newline

5chmidti

LGTM

+- some people may want to control this behavior and keep the explicit cast for enums, WDYT?

felix642 · 2024-11-13T14:02:21Z

+- some people may want to control this behavior and keep the explicit cast for enums, WDYT?

I can see why some people might want to explicitly cast the enum to its underlying type.
I could add the option WarnEnumCastToUnderlyingType which could default to true.

5chmidti · 2024-11-13T19:21:31Z

+- some people may want to control this behavior and keep the explicit cast for enums, WDYT?

I can see why some people might want to explicitly cast the enum to its underlying type. I could add the option WarnEnumCastToUnderlyingType which could default to true.

That sounds good, or, in line with the other options: IgnoreCastingEnumToUnderlyingType

HerrCai0907

Is it really a false negative? In another word, does enum E1 : char {}; mean E1 is the type alias of char?
IMO is not. for me a new option is more suitable for this case.

5chmidti · 2024-11-16T08:15:56Z

Is it really a false negative? In another word, does enum E1 : char {}; mean E1 is the type alias of char?
IMO is not. for me a new option is more suitable for this case.

I think it is a false negative in the sense that the cast is redundant, but it is a type conversion, so maybe the default should not warn on explicit casts from enums, and make the loss of explicit Information opt-in.

felix642 · 2024-11-19T02:43:09Z

@5chmidti and @HerrCai0907 I was revisiting this PR and I just remembered that my changes will only warn if IgnoreTypeAliases is set to true.
The description for this option is :

when set to true, it will resolve all type aliases and operate on the underlying types.

which, from my understanding, should cover enums.
I don't think we should add a second option to cover enums. What do you think ?

HerrCai0907 · 2024-11-19T04:19:31Z

I am not the expect of cpp standard, in my opinion, enum is not a type alias. it is a new type.

PiotrZSL

Please implement this change under separate option like: AllowEnumsBeRecognizedAs Aliases or something similar.
Implicit conversion between enums and underlying type is not always welcome by developers.

PiotrZSL · 2024-11-19T10:21:06Z

clang-tools-extra/test/clang-tidy/checkers/readability/redundant-casting.cpp

+enum class E2 : char {};
+
+void testEnum(E1 e1, E2 e2){
+  char a = static_cast<char>(e1);


That isn't a redundant cast. Such casts should be done in explicit way. In C++ is even better to use:
std::to_underlying since C++23 or std::underlying_type now.

felix642 · 2025-01-03T19:16:33Z

Closing this PR since I no longer think this is an option that we need.

llvmbot added clang-tools-extra clang-tidy labels Oct 7, 2024

felix642 force-pushed the I111137-readability-redundant-casting-false-negative-enum branch from 310315a to 9c28fc5 Compare October 7, 2024 19:35

[clang-tidy] Improved readability redundant casting with enums

a786f62

Fixed false negatives with readability-redundant-casting when the underlying types are the same and the option IgnoreTypeAliases is set to true. Fixes llvm#111137

felix642 force-pushed the I111137-readability-redundant-casting-false-negative-enum branch from 9c28fc5 to a786f62 Compare October 7, 2024 19:53

EugeneZelenko reviewed Oct 8, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

EugeneZelenko requested review from 5chmidti, HerrCai0907, PiotrZSL and SimplyDanny October 8, 2024 13:58

felix642 added 2 commits November 2, 2024 12:02

fixup! [clang-tidy] Improved readability redundant casting with enums

af97ade

Code Review & Removed non-portable test

felix642 requested a review from EugeneZelenko November 2, 2024 16:14

EugeneZelenko reviewed Nov 3, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

fixup! [clang-tidy] Improved readability redundant casting with enums

4362031

Removed excessive newline

felix642 requested a review from EugeneZelenko November 3, 2024 16:39

5chmidti approved these changes Nov 9, 2024

View reviewed changes

HerrCai0907 reviewed Nov 14, 2024

View reviewed changes

PiotrZSL requested changes Nov 19, 2024

View reviewed changes

felix642 closed this Jan 3, 2025

[clang-tidy] Improved readability redundant casting with enums #111424

[clang-tidy] Improved readability redundant casting with enums #111424

Uh oh!

Conversation

felix642 commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

5chmidti commented Oct 8, 2024

Uh oh!

felix642 commented Nov 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

felix642 commented Nov 13, 2024

Uh oh!

5chmidti commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HerrCai0907 left a comment

Choose a reason for hiding this comment

Uh oh!

5chmidti commented Nov 16, 2024

Uh oh!

felix642 commented Nov 19, 2024

Uh oh!

HerrCai0907 commented Nov 19, 2024

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrZSL Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

felix642 commented Jan 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

felix642 commented Oct 7, 2024 •

edited

Loading

llvmbot commented Oct 7, 2024 •

edited

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading

felix642 commented Nov 2, 2024 •

edited

Loading

5chmidti commented Nov 13, 2024 •

edited

Loading